Skip to content

Songpal code and test improvement#35318

Merged
cgarwood merged 13 commits into
home-assistant:devfrom
shenxn:songpal-improve
May 10, 2020
Merged

Songpal code and test improvement#35318
cgarwood merged 13 commits into
home-assistant:devfrom
shenxn:songpal-improve

Conversation

@shenxn
Copy link
Copy Markdown
Contributor

@shenxn shenxn commented May 7, 2020 •
edited
Loading

Breaking change

To call songpal/set_sound_setting on all songpal devices, the entity_id now need to be set to all instead of left unset.

Proposed change

Following reviews by @MartinHjelmare in #34714, here are some code improvements including the test import and service registration.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@shenxn shenxn mentioned this pull request May 7, 2020
Merged
8 tasks
MartinHjelmare
MartinHjelmare approved these changes May 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good!

The test failure is unrelated.

Comment thread homeassistant/components/songpal/media_player.py Outdated
@shenxn
Copy link
Copy Markdown
Contributor Author

shenxn commented May 7, 2020

I'm still working on adding some tests for the service calls. Should be done today.

@shenxn shenxn requested a review from MartinHjelmare May 7, 2020 18:52
MartinHjelmare
MartinHjelmare requested changes May 7, 2020
View reviewed changes
Comment thread tests/components/songpal/test_media_player.py Outdated
Comment thread tests/components/songpal/test_media_player.py Outdated
MartinHjelmare
MartinHjelmare reviewed May 7, 2020
View reviewed changes
Comment thread homeassistant/components/songpal/media_player.py Outdated
Comment thread homeassistant/components/songpal/media_player.py Outdated
Comment thread homeassistant/components/songpal/media_player.py Outdated
MartinHjelmare
MartinHjelmare reviewed May 8, 2020
View reviewed changes
Comment thread tests/components/songpal/test_media_player.py Outdated
Comment thread tests/components/songpal/test_media_player.py Outdated
MartinHjelmare
MartinHjelmare approved these changes May 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@shenxn
Copy link
Copy Markdown
Contributor Author

shenxn commented May 8, 2020 •
edited
Loading

@MartinHjelmare I think the integration is now good for "gold" integration quality. Can you double check if I missed anything? Also, I'm not pretty sure what the "Uses aiohttp and allows passing in websession" means in the "platinum" quality scale. Do you have any idea where I should look at?

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented May 8, 2020

Please copy the bullet list of the integration quality scale here and make a comment for each criteria. All may not apply etc.

https://developers.home-assistant.io/docs/integration_quality_scale_index

rytilahti
rytilahti reviewed May 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rytilahti rytilahti left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did just a very brief read-through and to me this looks good (I'm not knowledgeable enough to judge the test improvements, but more testing the better), thanks for the improvements!

About the removal of polling; this will/may break it for some users, as not all devices support the websocket interface. Are you sure we want to do that just to gain "gold status"? Alas, I cannot recall the details, but I think I added support for xhrpost (rytilahti/python-songpal@4b96e30) after getting some feedback just after the initial inclusion to homeassistant. It could have also been that the "xhrpost-only" devices were only sony bravia televisions which are unsupported anyway, so maybe this doesn't affect any real users..

Comment thread homeassistant/components/songpal/media_player.py Outdated
@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented May 8, 2020

Also, I'm not pretty sure what the "Uses aiohttp and allows passing in websession" means in the "platinum" quality scale.

This means that the backend library should be modified to allow passing aiohttp.ClientSession object to be used by the backend, or that's my understanding at least.

@shenxn
Copy link
Copy Markdown
Contributor Author

shenxn commented May 8, 2020

@rytilahti The poll is removed from __init__ simply because it is never used. The SongpalDevice was instantiated using SongpalDevice(name, endpoint) so poll is always False. If we do need to support polling for some devices, maybe we can add that feature in another PR.

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented May 8, 2020

Ohhhkay, no need to support polling then at all, all the better! I suppose (or hope) that the polling information got read from the config file at some point in the past, though..

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented May 8, 2020 •
edited
Loading

If the library uses aiohttp it should accept an (optional) client session, to meet the criteria, yes.

@shenxn
Copy link
Copy Markdown
Contributor Author

shenxn commented May 8, 2020 •
edited
Loading

Silver 🥈

  • Connection/configuration is handled via a component Not sure about what this means.
  • Set an appropriate SCAN_INTERVAL (if a polling integration) Not polling.
  • Raise PlatformNotReady if unable to connect during platform setup (if appropriate)
  • Handles expiration of auth credentials. Refresh if possible or print correct error and fail setup. If based on a config entry, should trigger a new config entry flow to re-authorize. No authentication.
  • Handles internet unavailable. Log a warning once when unavailable, log once when reconnected. Not pretty sure about this one. But since we connect to the device directly, I think we don't need to handle this separately.
  • Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected. Handled and there is test for that.
  • Set available property to False if appropriate Unavailable when disconnected. There is test for that.
  • Entities have unique ID (if available) MAC address is used as unique ID.

Gold 🥇

  • Configurable via config entries
    • Don't allow configuring already configured device/service (example: no 2 entries for same hub) Checked using endpoint.
    • Tests for config flow
    • Discoverable (if available SSDP discoverable
    • Set unique ID in config flow (if available)
  • Entities have device info (if available)
  • Tests for fetching data from the integration and controlling it
  • Has a code owner
  • Entities only subscribe to updates inside async_added_to_hass and unsubscribe inside async_will_remove_from_hass
  • Entities have correct device classes where appropriate
  • Supports entities being disabled and leverages Entity.entity_registry_enabled_default to disable less popular entities Only one entity so default enabled.
  • If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry. If I understand this correctly, the device/service API cannot remove entities for this integration.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented May 8, 2020 •
edited
Loading

Box number one means that the config should be centralized with either a config schema in the base integration module and/or by using config entries. So you can check that one too.

MartinHjelmare
MartinHjelmare approved these changes May 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gold level is approved. 🎉

@shenxn
Copy link
Copy Markdown
Contributor Author

shenxn commented May 8, 2020 •
edited
Loading

@MartinHjelmare @rytilahti Thanks a lot for the review. 👍

@cgarwood cgarwood merged commit 8994931 into home-assistant:dev May 10, 2020
@lock lock Bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rytilahti rytilahti rytilahti left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

@rytilahti rytilahti

Labels

breaking-change by-code-owner cla-signed code-quality has-tests integration: songpal small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shenxn @MartinHjelmare @rytilahti @cgarwood @homeassistant